@jimhester, @kevinushey Do you guys know if this change will cause problems on platforms that don't support C++17?

Yes, it definitely will, the CXX17 macro won't be set with the current Windows toolchain for instance.

Well actually this change doesn't actually affect Windows, since this only changes Makevars and not Makevars.win, but the underlying issue is the same, if R wasn't built with a compiler that supports C++17 then the CXX17 macro won't be set and the install will fail. This is why travis is failing for this PR for instance https://travis-ci.org/github/r-lib/later/jobs/667382735#L1790

Yes, I would stick with CXX11 and allow some way to opt-in to using CXX17 if supported by the active compiler. (This unfortunately does imply writing a configure script but it looks like later already has one so you've already accepted that pain)

Thanks, guys. There already is a configure script, so that at least reduces the amount of work needed.

Finally I spent some time trying to improve the configure script.

With this fix I can compile without issue in OpenBSD/adJ 6.6.

In this platform, the test program that I included in configure if compiled with CXX11 gives:

$ c++ -std=c++11 -c test_timespec.cpp
test_timespec.cpp:9:21: error: use of undeclared identifier 'TIME_UTC'
1 error generated.

but compiled with CXX17 gives no error:

$ c++ -std=c++17 -c test_timespec.cpp

@wch I would appreciate if you could please check.

Blessings.

@vtamara I don't think the test is correct -- on the Ubuntu CI, you can see that it says that C++17 is required, but that hasn't been true in the past. It's probably because it normally would use -std=gnu++11.

Finding the default compiler flags used by R for C++11 can be done by calling:

R CMD config CXX11FLAGS
R CMD config CXX11STD

However, keep in mind that not all platforms that we need to support have C++11 compilers, notably RedHat/Centos 6.

@wch What do you think about this one? I tested in OpenBSD/adJ 6.6 as well as Ubuntu 19.10 and looks good. Please feel free to change it to make it more portable.

@vtamara I've given some feedback, but don't currently have time to test and fix on various platforms for you. I don't want to introduce breakage on platforms that are widely used, or which CRAN requires us to build on. Notable platforms include Mac, Windows, RHEL/Centos 6 (which by default does not have a compiler that supports C++11), and Solaris (which has a very outdated toolchain but which CRAN still uses for checks.)

One more thought: it may be simpler to do something like this:

• If C++17 is available, use CXX_STD=CXX17
• else use CXX_STD=CXX11

@wch I implemented the simpler method you suggested.

For me it works on OpenBSD/adJ 6.6 with R 3.6.1, on Ubuntu 19.10 with R 3.6.1 and on Ubuntu 18.04 with R 3.4.4.

The CI shows it works fine on macOS-latest, windows and ubuntu-16.04 with R 3.6.x, although it has problems on ubuntu-16.04 and R 3.5, 3.4, 3.3 and 3.2.

On the platforms where it doesn't work it shows:

* creating vignettes ... ERROR
Error: processing vignette 'later-cpp.Rmd' failed with diagnostics:
'vignetteInfo' is not an exported object from 'namespace:tools'

So I guess, more things should be tested before setting CXX_STD=CXX17.

Any suggestion on how to test on those platform where it fails or on solaris?

I still think the logic isn't quite right. I think it should be something like this:

• If a C++17 compiler is available, we know that <ctime> must provide timespec_get and TIME_UTC (as per https://en.cppreference.com/w/cpp/chrono/c/timespec_get). In this case, we can just use CXX_STD=CXX17, without compiling any test code.
• If a C++17 compiler is NOT available, then <ctime> does not necessarily provide timespec_get and TIME_UTC -- but on all platforms we have actually encountered, it does. So we can just use CXX_STD=CXX11 in this case.

A bit more detail: in the second case (where C++17 is not available), rather than simply using CXX_STD=CXX11, it would be more correct to try to compile the test program to check for timespec_get and TIME_UTC. But again, we haven't actually encountered a platform where this is needed.

I believe that to check for C++17 support, you can run R CMD config CXX17 and check if it's empty, though I'm not 100% sure that's the right way. Documentation is here: https://cran.r-project.org/doc/manuals/R-exts.html#Using-C_002b_002b14-and-later

Just to add a bit more detail of the challenges for platform-specific stuff like this: we need to support not only the platforms that I mentioned earlier (and which we test with continuous integration) but also platforms with older versions of R, and older compilers. I noticed when I read this section of Writing R Extensions that with R 4.0, you can always assume a C++11 compiler is available; however, for older versions of R, that isn't the case.

@wch As far as I know, in configure scripts it is not advisable to check for tools and version but if the existing tools have the features required. I think this example and the CI results show why.
Although in Ubuntu 16.04 with R 3.5, it seems there is support for C++17, there are problems using that compiler. Also the documentation you reference says in the paragraphs about C++17 "Feature tests can be used (and probably should be as support is still patchy, especially library support)."

Since in most platforms it works with CXX_STD=CXX11, in my humble opinion it is better to change to CXX_STD=CXX17 only when test scripts for every problematic feature pass (of course identifying test script for those problematic features).

I understand your point of view, in trying to be conservative about the changes. If that's the idea, then this logic makes sense to me:

• First try to compile the test program using the CXX11 compiler, if available, and if not, the CXX compiler. (See my other comment about the weird way that R uses CXX_STD=CXX11.)
  • If it passes, then use CXX_STD=CXX11.
  • If the test program fails, then check if a CXX17 compiler is available.
    • If it is available, use CXX17.
    • If not available, Then fall back to CXX11

As I understand your code, it first looks for a C++17 compiler, and if present, it tries to compile a test program that should always pass according to the C++17 spec. The test program is therefore redundant (modulo the possibility of a non-compliant compiler, but the time stuff seems like a pretty basic part of the standard so that seems very unlikely.)

But the current problem with OpenBSD is that we try to use C++11, but that standard doesn't guarantee that the time stuff is available. The test program can fail when compiling the code with a C++11 compiler (this is basically what's happening with the current release version of later), but not a C++17 compiler. However, OpenBSD does have a C++17 compiler that does make the time stuff available.

A few other comments:

• The CI failures in older versions of R are due to some external problems, probably with the setup of the CI system. The tools::vignetteInfo function is only available from R 3.6.0 and up, so it shouldn't even be trying to use it. (See https://hughjonesd.shinyapps.io/rcheology/)
• In the context of a configure script for an R package, I think it's reasonable to check for CXX17. (I also just realized that "${R_HOME}"/bin/R CMD config CXX17 is what should be used to find the compiler to build the test program, since it also uses CXX17STD and CXX17FLAGS.) The time stuff seems like a pretty basic part of the standard, so I would expect it to work.
• I made some comments on the code that haven't been addressed.

Just a heads up: my time is very, very limited these days, and my other responsibilities mean that I can't really spend much more time on this.